Repository navigation
Register the doc-level query and job lock indices as system indices - #2320
DarshitChanpura wants to merge 3 commits into
Conversation
…indices Alerting creates and owns .opensearch-alerting-queries* (the default doc-level query alias and its -000001 and rollover indices) and .opensearch-alerting-config-lock (the job lock index), but did not register them, unlike its alert, findings, comments and config indices. Add both to getSystemIndexDescriptors() from the existing constants. The query pattern does not cover query indices that a caller names in a monitor's data sources. Neither pattern overlaps another alerting pattern or one registered by the other plugins in the distribution. Add a unit test for the patterns and a secured IT for direct access, and move the ITs that wrote the lock index or created and closed the query index directly to the admin certificate client. Signed-off-by: Darshit Chanpura <dchanp@amazon.com>
The direct write tests expected a 403 but did not show that the same user can write elsewhere, so a missing role permission would also pass. Write the same document to an ordinary index first and expect 201. Also state precisely which direct operation on the query index the security plugin refuses in the create monitor clean up test. Signed-off-by: Darshit Chanpura <dchanp@amazon.com>
PR Reviewer Guide 🔍(Review updated until commit 234158e)Here are some key observations to aid the review process:
|
PR Code Suggestions ✨Explore these optional code suggestions:
|
Codecov Report✅ All modified and coverable lines are covered by tests. ❌ Your project check has failed because the head coverage (67.83%) is below the target coverage (70.00%). You can increase the head coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #2320 +/- ##
============================================
+ Coverage 67.72% 67.83% +0.10%
- Complexity 105 1815 +1710
============================================
Files 160 170 +10
Lines 10343 14324 +3981
Branches 1522 2090 +568
============================================
+ Hits 7005 9716 +2711
- Misses 2672 3726 +1054
- Partials 666 882 +216 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
The create monitor clean up test in SecureMonitorRestApiIT failed on the secured CI run because monitor creation no longer failed. On main, the createIndex call created .opensearch-alerting-queries-000001 without the .opensearch-alerting-queries alias: its mapping string closed the request body before the aliases part, which the server ignored. With no write index behind the alias, updateQueryIndexMappings fails, so monitor creation fails and the test checks the clean up. The admin certificate body added in this PR also created the alias. A closed index behind the alias does not fail monitor creation: the mapping update succeeds on a closed index, and the test monitor has no queries to index. The test fails the same way with or without the new system index descriptors, so the descriptors do not change this behaviour. Create the same index, mappings and settings as main did, without the alias, still with the admin certificate. Signed-off-by: Darshit Chanpura <dchanp@amazon.com>
|
Persistent review updated to latest commit 234158e |
Description
Problem: Alerting registers its alert, findings, comments and config indices as system indices, but not two other indices it creates and owns:
.opensearch-alerting-queries*, the default doc-level query index..opensearch-alerting-queriesis an alias over.opensearch-alerting-queries-000001and its rollovers (DocLevelMonitorQueries)..opensearch-alerting-config-lock, the job lock index (LockService.LOCK_INDEX_NAME).So with the security plugin on, index permissions on these names allow direct reads and writes of monitor queries and job locks, which the other alerting indices do not allow.
Change: Add both to
getSystemIndexDescriptors(), built from the existing constants:The query pattern covers the alias name,
-000001and every rollover. It does not cover query indices that a transport caller names in a monitor'sdata_sources(security-analytics uses.opensearch-sap-<log type>-detectors-queries); those stay ordinary indices, as discussed in the issue.Overlap: core refuses to start a node when two system index patterns overlap. Neither pattern overlaps alerting's other four, and neither overlaps the patterns registered by job-scheduler, security-analytics, anomaly-detection, index-management, notifications, ml-commons, security, flow-framework, observability, reporting, k-NN, sql, cross-cluster-replication, asynchronous-search, search-relevance, geospatial or learning-to-rank on their current main branches. This was checked by passing all of them to core's
SystemIndices, which runs the same check as node start.Upgrade: nothing is migrated. On restart the cluster manager marks existing indices that match the new patterns as system indices in their metadata; the data and mappings do not change. After that:
system_readandsystem_writethread pools, so the percolate search of doc-level monitors no longer runs on thesearchpool, and concurrent segment search is not used for it.all_access. Direct writes, deletes, mapping and alias changes and close need the admin certificate (orsystem:admin/system_indexwhen system index permissions are enabled); direct searches return no hits. This includes a query-level monitor whose input matches these indices by a wildcard. Alerting's own access is unaffected, because it reaches these indices without a user attached, in the same flows that already write the registered alert, findings and config indices.Open question, for the secured CI run: a few alerting calls on these two indices do not stash the context themselves, right after a step that restores the caller's context:
deleteLockinDeleteMonitorService, afterTransportDeleteMonitorActionrestores the caller's context in its coroutine;indexDocLevelMonitorQueriesand the query delete-by-query inTransportIndexMonitorAction, after the same restore. The config index writes beside them use the*Stashedhelpers.The coroutine usually resumes on a worker thread without a user, and secured runs on main where users without permissions on these names create and delete doc-level monitors point that way, but the code does not guarantee it. If the caller's user is present, these calls are now refused for
all_accessas well as for users without index permissions: a refused lock delete fails the delete request after the monitor document is gone, a refused query cleanup is only logged, and an update can leave the monitor and its queries out of step. This PR does not change those paths; running them with the plugin's own context is part of the planned move fromstashContext()to the plugin client. The scheduled run, the lock acquire and release, and the percolate search run in the same context as the alert and findings writes that already work.Release note: With the security plugin enabled, the default doc-level query indices (
.opensearch-alerting-queries*) and the job lock index (.opensearch-alerting-config-lock) are now system indices, like the alert, findings and comments indices. Users can no longer read or write them directly. Clusters without the security plugin are unaffected.Testing
AlertingPluginSystemIndicesTests(unit): the alias name,-000001,-000002and the lock index each match exactly one descriptor; a security-analytics query index name matches none; and no two alerting descriptors overlap, the same check core runs at node start. Removing either descriptor, dropping the*from the query pattern, adding an overlapping descriptor, or widening the query pattern each fails the matching test. Both index tests fail on main.SecureQueryAndLockIndexRestApiIT(runs with-Dsecurity=true): a user with read and write permissions on every index gets no hits from a direct search of each index, while the admin certificate finds the documents, and gets a 403 on a direct write, while the same user can write the same document to an ordinary index. It followsSecureFindingsRestApiIT.DocumentMonitorRunnerIT, renewable and non-renewable lock tests) or created and closed the query index (SecureMonitorRestApiIT, create monitor clean up test) through the admin user now use the admin certificate, ascreateFindingdoes for the findings index. The clean up test keeps the state it had on main: the oldcreateIndexcall created.opensearch-alerting-queries-000001without the.opensearch-alerting-queriesalias, because its mapping string closed the request body before thealiasespart. With no write index behind the alias,updateQueryIndexMappingsfails with "Failed to get write index for queryIndex alias", and monitor creation fails and cleans up. The new body creates the same index, mappings and settings, again without the alias. A closed index behind the alias does not fail creation: the mapping update succeeds on a closed index, and the test monitor has no queries to index. That is the same with and without this change../gradlew ktlint :alerting:testpasses (476 tests, 4 new, 0 failures; 472 on main). On a secured cluster (-Dsecurity=true -Dhttps=true): the clean up test fails with the alias in the setup, with or without the two new descriptors, and passes with the setup above;SecureQueryAndLockIndexRestApiITpasses (4 tests), andSecureMonitorRestApiITpasses 63 of 64. The one local failure,test create PPL monitor fails when user lacks index permissions, is aNoClassDefFoundErrorinside the SQL plugin build on the local cluster and does not involve these indices. The unsecured integration tests have not been run locally and are left to CI.Related Issues
Related: #2318
Check List
--signoff.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.